Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.86%. Comparing base (ff7a485) to head (3958649).

@@            Coverage Diff             @@
##             main    #9862      +/-   ##
==========================================
- Coverage   91.88%   91.86%   -0.03%     
==========================================
  Files         360      360              
  Lines       16728    16732       +4     
==========================================
  Hits        15371    15371              
- Misses       1020     1022       +2     
- Partials      337      339       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Blocked on open-telemetry/opentelemetry-collector-contrib#31802

It would also be interesting to try and reproduce the failing contrib test on a unit test here, so we can work on it. The offending Unmarshal function is here https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/e4c5b51ec0e24ca163bcb64d944455577f90a09f/pkg/stanza/operator/helper/time.go#L49

It would also be interesting to try and reproduce the failing contrib test on a unit test here, so we can work on it. The offending Unmarshal function is here https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/e4c5b51ec0e24ca163bcb64d944455577f90a09f/pkg/stanza/operator/helper/time.go#L49

added a skipped test here: TestRecursiveUnmarshaling

If you want a foolproof solution, we might have to do something a little more drastic. We might need to keep a hashset of all pointers of unmarshaler methods ever called, to make sure we don't run into cycles.

If you want a foolproof solution, we might have to do something a little more drastic. We might need to keep a hashset of all pointers of unmarshaler methods ever called, to make sure we don't run into cycles.

I guess we can forbid what the stanza package does instead. Do you think there is a benefit to being able to do what stanza does? I can't see any advantage right now

No, I don't see any advantage, except maybe if you're trying to unmarshal and set all values on the struct atomically. Even then, this is a corner case. I think we're ok.

One more PR in contrib needed: open-telemetry/opentelemetry-collector-contrib#32577

The last contrib issue is solved by open-telemetry/opentelemetry-collector-contrib#32660. Unfortunately, I couldn't find a good way to fix this - in effect, a breaking change.

@atoulme are these expected:

    config_warnings_test.go:92: 
        	Error Trace:	/tmp/opentelemetry-collector-contrib/exporter/datadogexporter/config_warnings_test.go:92
        	Error:      	elements differ
        	            	
        	            	extra elements in list B:
        	            	([]interface {}) (len=1) {
        	            	 (string) (len=125) "\"metrics::histograms::send_count_sum_metrics\" has been deprecated in favor of \"metrics::histograms::send_aggregation_metrics\""
        	            	}
        	            	
        	            	
        	            	listA:
        	            	([]string) (len=1) {
        	            	 (string) (len=125) "\"metrics::histograms::send_count_sum_metrics\" has been deprecated in favor of \"metrics::histograms::send_aggregation_metrics\""
        	            	}
        	            	
        	            	
        	            	listB:
        	            	([]string) (len=2) {
        	            	 (string) (len=125) "\"metrics::histograms::send_count_sum_metrics\" has been deprecated in favor of \"metrics::histograms::send_aggregation_metrics\"",
        	            	 (string) (len=125) "\"metrics::histograms::send_count_sum_metrics\" has been deprecated in favor of \"metrics::histograms::send_aggregation_metrics\""
        	            	}
        	Test:       	TestSendAggregations/metrics::histograms::send_count_sum_metrics_set_to_false

@atoulme So that we can merge this soon and address @codeboten's comment, would you be able to do the following?

1. File a draft PR on opentelemetry-collector-contrib that pulls in this version of opentelemetry-collector + adds the necessary fixes in contrib. This is to 'prove' that we can satisfactorily fix the issue in contrib
2. Post the PR from (1) here. Once this is done, we can merge this PR.
3. Update contrib and mark the PR from (1) as ready to review with only the fixes

Here is the PR open-telemetry/opentelemetry-collector-contrib#32660

It is past midnight, I will follow up tomorrow on it.

The contrib PR shows the changes combined with test changes to contrib will work. I think we have reached step 2. I have reached out to Alex to make sure he's ok with this.

@codeboten are you ok with this approach and the steps I have followed, as well as the checks they provide? Can we merge this PR?

Can we agree to merge this by end of next week unless someone objects?

I'll put a reminder to merge in a wekk, @codeboten feel free to merge before then if you are okay with it :)